-
Notifications
You must be signed in to change notification settings - Fork 234
Add new exception GMTParameterError for invalid parameters #4003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cc89cb1 to
a4082e0
Compare
cf33edc to
e51bb31
Compare
| if required: | ||
| msg = ( | ||
| "Required parameter(s) are missing: " | ||
| f"{', '.join(repr(par) for par in required)}." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case where the parameter(s) is required/compulsory, I'm wondering if we can move towards having the error being thrown natively by Python, instead of writing code like:
if kwargs.get("F") is None:
raise GMTParameterError(required={"filter_type"})Should we revisit #2896 which mentions PEP692, and also think about using PEP655's typing.Required qualifier (https://typing.python.org/en/latest/spec/callables.html#required-and-non-required-keys)? Maybe a step towards #262 is to disallow single character arguments for required parameters, so that we can have native Python errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue is that, with the new alias system #4000 implemented, **kwargs will no longer be needed. We will still keep **kwargs to support single-letter option flags, but other parameters won't be passed via kwargs anymore.
947d3f4 to
c2c81d7
Compare
No description provided.